good to review

Can you explain why there was a *Sevice in the first place?

Because for me the serviceName (previous service) should be completely removed from task/event struct.

Why Task/Event have to have knwoleage of service they are in? (Besides logs ofc)

If the logs are only reason, then info about task/event services should be added somewhere else, not on struct level

@krhubert Service info used while producing errors like here

yes, but besides errors/logs?

If the errors/logs are the only reason, then info about task/event services should be added somewhere else, not on struct level

So if for errors, then it should be removed, and error should be without ServiceName, or the service name should be added by executor of the task/event

the code is ok, but changing from *Service to string is cosmetic, it should be removed at all.

@krhubert I understand but the change is not for cosmetic reasons. we had these cycling references before between Service type and others and now we don't keep pointers around. I don't quite agree on removing this meta data because they provide more detailed info about the errors. We can also get these meta info from the caller's level but it'll require us to create an additional error to combine all info about the error. /cc @mesg-foundation/core any more feedbacks?

I agree that these structures shouldn't be aware of their "parent" structure especially if they don't need anything from their parent for their business logic. A structure should be self contained as much as possible and I agree that it adds some context for the errors, context that we need but the question to ask is who is responsible for these errors is it the task itself or for example the api that has the wrong parameters and so make this task invalid. The task should just say if it's valid or no and no more. The api in this example should provide the detail and the context of the error (even if we need to wrap the task error into another error).

I'm totally pushing to remove these kind of dependencies that I feel are not needed and might create a lot of problem to maintain the product.

I feel it's always good to avoid this kind of situation where we have a double dependency and keep it only if there is really no way we can avoid that (even this I'm sure there is always ways to avoid that).

Also I think (but didn't try yet) that you introduced a bug in the fromService function, you removed the service but didn't add the serviceName or taskKey and this is exactly the kind of problems I'm afraid with this double dependency but I might be wrong on this example, I didn't test it yet.

In the end I'm more than agree that the goal for that is not to pass the name instead of the whole service/task but instead really remove the notion of service/task where we don't need it.

about the removed lines from fromService() it should not introduce any bugs, they are just forgotten there, Getters sets the needed values. tests are also passes, if you see any issues please let me know.

for keeping the parent's meta data info in the child, i think this is a grey area, even std packages introduces this kind of stuff. there should be a balance between where to do this or not. to me, this one is not "evil" and ok. but i'm ok with the both ways. we can combine the errors in the caller's side but we should not modify the returned error

I'm not sure I really understand the part where it's removed from the fromService but it's ok because the getters set the value.

What happen if we never call the getters ? In this case we will not have the service/serviceName. This can be a bit dangerous no ?

the whole idea of exposing service definition from Service type is a bit ambiguous to me. I'd rather having a Definition struct and keep it under Service type privately. While refactoring service package, I didn't choose do this to reduce the complexity of PR and leave it as it is to experiment with it a bit and later decide for an improvement. I was thinking about having a Definition type where it has all these service definition and keeps the values without using pointers, this way it also prevents cycling references.

we don't have any piece of logic that depends on accessing Service definitions directly instead of using Getters and I think that will never change. I agree with @antho1404 that it's a bit ambiguous to not have serviceName when you directly access it from Service type's fields.

lets discuss what direction we should go with, I suggest the first option below:

• let FromService() to get a Definition and have a private def Definition field under Service type. Keep Service reference in Task, Output etc. (Service field will not have a reference to these types, so it'll not cause to cycling refs).
• set serviceName etc inside fromService(), optionally rm them completely.

with the first option we can also get rid of cycling ref for Dependency type too.

• let FromService() to get a Definition and have a private def Definition field under Service type. Keep Service reference in Task, Output etc. (Service field will not have a reference to these types, so it'll not cause to cycling refs).

I'm not sure to understand. Definition will be a new struct?

I suggest to get rid of any reference (*Service and even Service Name).
Task, Events, Parameters don't need Service in order to work properly.
For the error message, yes the message itself will not say which service it is. But we can catch this type of error and convert them to errors than have more context (like the service name or ID).

Example with the unmarshal function in database package that return a new error rather than the one returned from json.Unmarshal that have zero knowledge about the service id.
https://github.com/mesg-foundation/core/blob/c2527ae17bad7f57fe9b1fa8c4027c617bf214f2/database/service_db.go#L58-L64

by Definition type i'm talking about moving some parts of Service type to a new type:

current:

type Service struct {
	ID            string               `hash:"-"`
	Name          string               `hash:"name:1"`
	Description   string               `hash:"name:2"`
	Tasks         []*Task              `hash:"name:3"`
	Events        []*Event             `hash:"name:4"`
	configuration *Dependency          `hash:"-"`
	Dependencies  []*Dependency        `hash:"name:5"`
	Repository    string               `hash:"name:6"`
	DeployedAt    time.Time            `hash:"-"`
	statuses      chan DeployStatus    `hash:"-"`
	tempPath      string               `hash:"-"`
	docker        *container.Container `hash:"-"`
}

after:

type Service struct {
	ID         string               `hash:"-"`
	DeployedAt time.Time            `hash:"-"`
	def        Definition           `hash:"definition"`
	statuses   chan DeployStatus    `hash:"-"`
	tempPath   string               `hash:"-"`
	docker     *container.Container `hash:"-"`
}

type Definition struct {
	Name          string       `hash:"name:1"`
	Description   string       `hash:"name:2"`
	Tasks         []Task       `hash:"name:3"`
	Events        []Event      `hash:"name:4"`
	Configuration Dependency   `hash:"-"`
	Dependencies  []Dependency `hash:"name:5"`
	Repository    string       `hash:"name:6"`
}

//  FromDefinition. /cc @NicolasMahe
//
// This one can be used while creating tests otherwise we need to use New
// all the time and provide a tarball.
//
// Or we can completely lose this and have a helper to create a tarball from Definition type
// and use it with New in tests.
service.FromDefinition(Definition{
       Tasks: []Task{},
})

// To save service into database we might need to create custom Marshaler to save info in def field,
// otherwise it'll be ignored by json package because it's private.

// And it seems we need to still keep FromService -or an equivalent if we change
// the way how Service saved to database- (/cc @NicolasMahe) to upgrade Service
// read from database

please not that Definition does not hold any pointer values (prevents cycling refs).

• with this approach we can keep Service reference in the other types like Dependency and Task and this will not cause to a cycling reference because Definition type does not keep references of types like Dependency and Task.

• the second argument I hear that, a type (child) returned from a method (e.g. service.GetTask()) of other type (parent) shouldn't know parent's info nor should keep a reference to parent. i don't quite think that this is a bad style of programming. yes we can also, remove Service reference or serviceName field from types like Task and generate combined errors like @NicolasMahe suggested, it seems a bit more elegant but at the same time keeping a reference to parent to get more info about it is not something that I dislike. I'm fine with the both ways.

• as a reminder, removing Service reference from Dependency type is kinda challenging because some methods of Dependency type requires more info from Service type, if we adopt having the Definition type, we can avoid a cycling reference here too.

• by having Definition type we disallow directly accessing to definition info from Service type and encourage using Getters. But having a private Definition field can complicate saving Service field directly to database. This is a disadvantage. And callers from service package can also access definition directly from Service instead of getters, this cannot be prevented.